Improve CQL Injection Query #200

jeongsoolee09 · 2025-06-19T13:22:16Z

What this PR contributes

Improve CQL Injection Query.
- A new class CqlQueryRunnerCall now expands and replaces TaintedCqlClause, now covering cds.run, cds.db.run, srv.run, and tx.run.
  - Also, it models CRUD-style "shortcut calls" that translates to running a CQL on a service under the hood.
- Expand base objects as possible receivers of methods .run and property read entities:
  - EntityEntry is "absorbed" into EntityReference. Plus, EntityReference now covers more examples, namely, cds as a shortcut to cds.db.
- Make the alert message more useful by making the query itself the primary location which is different from sink (the string concatenation).
Add robust test cases (This is the gist of this PR, please take a look for the behavioral summary description of what this PR aims to implement).
- send11-send17: Service1 runs query on the database service using cds.run and friends.
- send21-send25: Service1 runs query on itself by await-ing the query.
- send31-send37: Service1 runs query on itself using this.run and friends.
- send41-send47: Service1 runs query on Service2 using Service2.run and friends.
- send5: Service1 runs query on Service2 using CQN parsed with cds.ql.
- send6: Service1 runs query on the database service using CQN parsed with cds.parse.cql.
- send7: Service1 runs query on the database service using CQN parsed with global function CQL.
- send81: Service1 runs query on Service2 using an unparsed CDL string (only valid in old versions of CAP).
- send91-send97: Service1 runs query on Service2 using Service2.tx( tx => tx.run(...) ) and friends.
- send101-send107: Service1 runs query on itself using this.tx( tx => tx.run(...) ) and friends.
- send111-send117: Service1 runs query on the database service using cds.tx( tx => tx.run(...) ) and friends.
- send121-send127: Service1 runs query on the database service using cds.db.tx( tx => tx.run(...) ) and friends.

Future works

The alert message can be more refined: if a sink is in cds.read(...).from(...), only the cds.read(...) part is alerted on, where the entire chained method call is more desirable as a alert location.
SensitiveExposure.ql seems to be quite brittle, it needs a rewrite. This PR only updates the query's reference to old definitions that are no longer available.

…idiomatic

Will fix later

github-advanced-security

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

… duplicate old test case

These are combined into the new CQL injection test file to avoid confusion; the few commits that precede this one "ports" the old test cases to the newer test file.

jeongsoolee09 · 2025-06-26T17:34:53Z

Added more test cases by moving over the test cases in the old cqlinjection.js (which was once moved to old/cqlinjection.js) and further split the test cases:

concatenation between a string literal and an expression: SELECT.fromEntity1.where("ID=" + id);
concatenation between a template literal and an expression: SELECT.fromEntity1.where(ID= + id);
concatenation between a template element and an expression: SELECT.fromEntity1.where(ID=${id});
Tagged template literal with an expression interpolated (NOTE: FP): SELECT.fromEntity1.whereID=${id};

lcartey

I've completed a partial review, with some initial comments.

javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll

javascript/frameworks/cap/src/cqlinjection/CqlInjection.ql

javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll

This predicate is (1) not used anywhere, and (2) nothing but a simple alias to `API::Node.asSource/0`.

The original only covered `StringLiterals`, but `getStringValue` covers more cases ("constant folding").

…FromCdsLib/0` As stated in the new docstring of `CdsFacade`, the same `cds_facade` object can be accessed either via `@sap/cds` or `@sap/cds/lib`. Therefore, there is no distinction to be made between the two. Also, the `CdsDb` and the `GloballyAccessedCdsDbService` aimed to capture the same thing, so delete the latter in favor of the former (it's clearer and more concise).

…g to it This avoids duplicating the sink definition in `getQueryOfSink` and also in the `isSink` of the configuration.

Now, "@sap/cds/lib" is also a valid import path of a `CdsFacade` object.

lcartey · 2025-07-01T10:07:55Z

...ript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll

+     * This step jumps from `id` in the property value expression to the enclosing object `{ id: "" + id }`.
+     * This in conjunction with the above step 2 will make the taint tracker jump from `id` to the entire
+     * INSERT clause.
+     */


Can you explain why we need this case? I would have expected an untrusted property value for id could not cause an injection vulnerability in this query.

As a real world example, adding this step introduces a new result in cloud-cap-samples:
https://github.com/SAP-samples/cloud-cap-samples/blob/main/reviews/srv/reviews-service.js#L21-L25
That looks to me like a false positive, but perhaps I'm missing something!

You're right, this step should only consider argument->returnvalue progagations where the argument is a string concatenation or originates from one.

I've addressed it in 689e00a. Now it no longer creates the FP alert on the reviews-service.js.

lcartey · 2025-07-01T10:11:46Z

javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll


  ServiceInstance getRunner() { result = srv }

  SourceNode getContextObject() {
-    result = this.getAnArgument().getALocalSource() and not result instanceof FunctionNode
+    /* 1. An object node passed as the first argument to a call to `srv.tx`. */
+    result = txCall.getAnArgument().getALocalSource() and not result instanceof FunctionNode


The comment says first argument, did you mean getArgument(0)?

The srv.tx method does not accept multiple context objects at once, and the callback function always comes after a context object argument, so technically this does what we want. However, it's still more accurate to just put down exactly what we want. 👍

javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll

lcartey · 2025-07-01T10:32:09Z

...ript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll

+ * concatenation expression. e.g.
+ * ``` javascript
+ * cds.read("Entity1").where(`ID=${id}`); // Notice the surrounding parentheses!
+ * cds.create("Entity1").entries({id: "" + id});


I don't think this case is covered by this class.

I'm very glad you pointed this out, create is indeed a hole in our coverage that needs to be modeled.

lcartey · 2025-07-01T10:32:59Z

...ript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPCqlInjectionQuery.qll

+ * A string concatenation expression included in a CQL shortcut method call. e.g.
+ * ``` javascript
+ * cds.read("Entity1").where(`ID=${id}`); // Notice the surrounding parentheses!
+ * cds.create("Entity1").entries({id: "" + id});


I don't think the entries cases in this qldoc are actually covered by the class.

This is related to our hole in the coverage for create, thus will be addressed together.

lcartey · 2025-07-01T10:46:38Z

javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js

+
+    this.on("send00144", async (req) => {
+      const { id, amount } = req.data;
+      cds.update("Entity1").set("col1 = col1" + amount).where`col1 = ${id}`; // FP


Why is this considered an FP? Also, is set permitted to have a string argument? The documentation says:

Specifies the data to update...

As a single-expression tagged template string

As an object with keys being element names of the target entity and values being simple values, query-by-example expressions, or CQN expressions:

Ouch, that's labeled as FP by mistake. The where clause is a tagged literal expression .where`col1 = ${id}` which if it were only by itself would have been an FP (SAFE) case. But the .set("col = col1" + amount) sitting there makes it a TP (UNSAFE) case.

lcartey · 2025-07-01T10:46:53Z

javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js

+
+    this.on("send00174", async (req) => {
+      const { id } = req.data;
+      cds.delete("Entity1").where`ID = ${id}`; // FP


Is this still an FP?

It's a bit misleading to mark a case as "FP" when what I really mean is this is a COMPLIANT case and thus not something our query should not alert on.

Maybe I should use "SAFE" / "UNSAFE" instead of "FP" / (no label). I'll address them in a commit.

lcartey · 2025-07-01T10:47:05Z

javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js

+    this.on("send00214", async (req) => {
+      const { id } = req.data;
+      const { Service1Entity } = this.entities;
+      await SELECT.from(Service1Entity).where`ID=${id}`; // FP


Is this still an FP?

It is an FP (it's safe). Same strategy as this comment will be used and will be addressed together.

lcartey · 2025-07-01T10:47:39Z

javascript/frameworks/cap/test/queries/cqlinjection/srv/service1.js

+    this.on("send00224", async (req) => {
+      const { id } = req.data;
+      const { Service1Entity } = this.entities;
+      await INSERT.into(Service1Entity).entries`ID = ${id}`; // FP


There's a number of other FP markers in this file that should be reviewed - I haven't higlighted them all individually.

It is an FP (it's safe). Same strategy as this comment will be used and will be addressed together.

lcartey · 2025-07-01T10:49:19Z

javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll

+  CallNode txCall;
+
+  CdsTransaction() {
+    txCall = srv.getAMemberCall("tx") and


This charpred either selects the tx call or the callback passed to the tx call - can you provide a quick example in the doc between the two? It's been very helpful where you've done that else where in the code.

lcartey · 2025-07-01T10:55:09Z

javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll

+          or
+          /* Imported from `cds.ql` */
+          exists(CdsFacade cds |
+            cds.getMember("ql").getMember(name).getAValueReachableFromSource().asExpr() = this


Does this have to be a VarRef?

lcartey · 2025-07-01T10:55:48Z

javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CQL.qll

-      /* Imported from `cds.ql` */
-      exists(CdsFacade cds |
-        cds.getMember("ql").getMember(name).getAValueReachableFromSource().asExpr() = this
+    exists(VarRef varRef | this = varRef |


The exists is currently superfluous. Did you mean to do something with different between varRef and this?

1. The second step should jump from a argument of a CQN object *only if the argument originates from a string concatentation*. Note that this new version identifies the end point using a successive application of `getAPredecessor`; it overapproximates and might accidentally include code that's not necessarily what we want. 2. The third is a specialization of the second step, and concerns itself only to the property writes to the object to be passed as an argument to the CQN query builder for INSERT and UPSERT.

Co-authored-by: Luke Cartey <[email protected]>

jeongsoolee09 added 4 commits June 17, 2025 12:57

Minor readability

ac0265e

Make definition of ImplMethodCallApplicationServiceDefinition more …

6fd6b2a

…idiomatic

Add srv.entities as part of EntityEntry

439ad43

Put CQL query-relevant definitions in a query library

3b7d741

jeongsoolee09 marked this pull request as draft June 19, 2025 13:22

jeongsoolee09 added 11 commits June 19, 2025 14:53

Do some refactoring

440712f

Sort out EntityReference

dfa8c08

Create a new CQL injection test project and move the old one to a folder

571b316

Minor numbering

1262260

install @sap/cds-dk and bump version of @sap/cds

908a572

Move existing CQL Injection test case to old/

a8a0bb4

Refine test cases and add more cases

5e39be5

Finalize working draft on current CqlInjection test case

25450a8

Finalize CqlInjection

0f95069

Fix test case and code

1e83f5a

Appease compiler to make SensitiveExposure pass

9a2e99b

Will fix later

github-advanced-security bot found potential problems Jun 24, 2025

View reviewed changes

jeongsoolee09 added 11 commits June 24, 2025 09:37

Debug isSink

c5c9740

Cover all cases in the current CQL injection test

89522d0

Get the query depending on the type of sink

43738d5

Minor formatting and tidying up

13691d4

Docstrings and comments

468a780

Fix a regression in taintedclause

a391d34

Update expected test outputs of old cqlinjection.js

39b2397

Fix a regression in applicationserviceinstance

fd0d13a

Fix regression in SensitiveExposure

621a319

Remove unneeded code

a0f6d9a

Update expected results of sensitive-exposure

7652149

jeongsoolee09 marked this pull request as ready for review June 24, 2025 23:08

jeongsoolee09 requested a review from lcartey June 24, 2025 23:08

jeongsoolee09 added 11 commits June 24, 2025 19:21

Fix numbering and remove duplicate case

2e56ae0

Make the new CQL injection test project a proper test case and remove…

a13a825

… duplicate old test case

Remove unneeded comment

8a99661

Checkpoint

5c1a5ba

Add some more unit test cases

a93a7b1

Add some more cases and add FP tags

d968632

Remove the old CQL injection test files

fb979d2

These are combined into the new CQL injection test file to avoid confusion; the few commits that precede this one "ports" the old test cases to the newer test file.

Add FP cases that are left out in the previous commit.

bdfc5bd

Debug query according to the new test cases

0004b88

Add expected test result of new CQL injection test

f0bb40c

Update test results of sensitive-exposure

8a6e11d

Update expected analysis results

18ef3b5

lcartey requested changes Jun 27, 2025

View reviewed changes

jeongsoolee09 added 5 commits June 30, 2025 08:02

Remove CdsFacade.getNode/0

cd26594

This predicate is (1) not used anywhere, and (2) nothing but a simple alias to `API::Node.asSource/0`.

Use getStringValue/0 in getNamespace/0

d7f5ed4

The original only covered `StringLiterals`, but `getStringValue` covers more cases ("constant folding").

Factor out sink definition in a class and associate location reportin…

5d52200

…g to it This avoids duplicating the sink definition in `getQueryOfSink` and also in the `isSink` of the configuration.

Add some comments and docstrings

caac1ae

jeongsoolee09 requested a review from lcartey June 30, 2025 13:50

jeongsoolee09 added 3 commits June 30, 2025 09:52

Minor documentation

97d6a01

Update test result

9c3138b

Now, "@sap/cds/lib" is also a valid import path of a `CdsFacade` object.

Tidy up code

378436a

lcartey requested changes Jul 1, 2025

View reviewed changes

lcartey reviewed Jul 1, 2025

View reviewed changes

jeongsoolee09 and others added 3 commits July 1, 2025 14:17

Explicitly use argument index in CdsTransaction.getContextObject/0

806e615

Improve writing of a QLDoc

dc42b54

Co-authored-by: Luke Cartey <[email protected]>

Improve CQL Injection Query #200

Are you sure you want to change the base?

Improve CQL Injection Query #200

Conversation

jeongsoolee09 commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR contributes

Future works

Uh oh!

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Uh oh!

jeongsoolee09 commented Jun 26, 2025

Uh oh!

lcartey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeongsoolee09 Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jeongsoolee09 commented Jun 19, 2025 •

edited

Loading

jeongsoolee09 Jul 1, 2025 •

edited

Loading